Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Support for using multiple databases in datastore #1090

Merged
merged 75 commits into from
Sep 29, 2023

Conversation

danieljbruce
Copy link
Contributor

This PR will add multi db support so that datastore users can interact with multiple databases.

Adds databaseId to the request options, client options and code for carrying over the client options to the request options as well as a test to make sure it actually works.
@product-auto-label product-auto-label bot added size: s Pull request size is small. api: datastore Issues related to the googleapis/nodejs-datastore API. labels Mar 20, 2023
The test should include additional checks to ensure get is running the way it is supposed to.
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Mar 20, 2023
Since we are saving things to another datastore we need to delete the data from that datastore.
The user needs a way to get the database Id from the client so that they can keep track of their clients more easily.
Ensure that the database id gets passed all the way down to the generated layer and gets included in the request.
We should check for the existance of datastore.options so that the code doesn’t fail if they are not provided at all.
@danieljbruce danieljbruce changed the title A simple test with multidb support feat: Support for using multiple databases in datastore Mar 20, 2023
The test on kokoro is failing because in that environment a project id isn’t defined. The solution is to eliminate the get request so that a call never reaches the backend and initialize the gapic client so it can be mocked out and then catch any call that reaches the mock.
Add docs comments for newly added functions and add a test that ensures setDatabaseId works correctly.
Don’t use the same string multiple times. When the database name we are using for testing changes then we should only have to make that change in one place.
This was a typo. We want to test against the database we were using before
Add the getRequestWithDatabaseId function and change it so that it can easily be used in multiple places and the tests still pass.
The request options should contain databaseId so rename the function as appropriate.
This commit contains a comment in all the places we want to add the reqOpts change.
Add a warning because the way the assert checking was done before, the test suite was not receiving any indication of which test was failing which makes debugging really hard.
Add request ids back in all the places they are required
Add addDatabaseIdToRequest to the transaction test framework so that it can be used in the tests.
Mock out add database id for the tests in createReadStream that correspond to createReadStream.
Add the addDatabaseIdToRequest mock to the tests for every runQueryStream test so that the tests pass.
addDatabaseIdToRequest should be a default function in datastore object of request so that we don’t have to mock it out in many different places.
Imports that were used in old mocks no longer exist in the file so should be removed.
Parameterized testing can be added to the test/index file to test to see how the tests behave with databaseId set to the second database and without this setting too.
Parameterized testing should be used on transaction.ts. This commit adds the testing with async.
Tests need to run on the default database as well as the secondary database. This commit ensures that they do.
Parameterized testing for the system tests will ensure that tests run with the default database as well as with the secondary database.
The intent of the datastore variable in the new tests is to be the default database so that we are able to see if reads/writes affect the default or secondary database in these tests. They should not use the parameterized version of datastore.
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Sep 20, 2023
@@ -50,19 +52,24 @@ const fakePfy = Object.assign({}, pfy, {
},
});

async.each(
[{}, {databaseId: SECOND_DATABASE_ID}],
(clientOptions: DatastoreOptions) => {
describe('Transaction', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we run the linter, this block will become indented and the PR will be hard to read.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, there's a setting in the GitHub interface to do just that. Clicking the small cog icon when in the "Files Changed" tab will open the popup where it's possible to select & click on "Hide whitespace > Apply and reload" option. Using that option will give you just the same type of view you have right now after you run the linter.
Screenshot 2023-09-27 at 4 39 15 PM

It's also a git command line option: git diff -w

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

system-test/datastore.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/request.ts Outdated Show resolved Hide resolved
Now that the tests don’t need to mock out adding database id function, we can inline the code that adds the database id to the request options.
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: l Pull request size is large. labels Sep 21, 2023
src/request.ts Outdated
@@ -1155,6 +1158,7 @@ export interface SharedQueryOptions {
};
}
export interface RequestOptions extends SharedQueryOptions {
databaseId?: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

discussed offline, will move databaseId to shared query options to match proto: example https://github.com/googleapis/googleapis/blob/master/google/datastore/v1/datastore.proto#L208-L234

system-test/datastore.ts Outdated Show resolved Hide resolved
Parameterized tests should have a different namespace for the second set of tests for sure so we add this prefix.
The databaseId should be moved to shared query options as discussed.
Copy link
Contributor

@ruyadorno ruyadorno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Linting was broken so that the PR could be read. Ran the linter again.
@product-auto-label product-auto-label bot added size: xl Pull request size is extra large. and removed size: m Pull request size is medium. labels Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: datastore Issues related to the googleapis/nodejs-datastore API. size: xl Pull request size is extra large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants